Skip to content

refactor: extract CSVImport._decompose_site_role, inverse of _evaluate_site_role - #1812

Draft
jacalata wants to merge 4 commits into
jac/csv-import-fixes-and-find-by-namefrom
jac/csv-site-role-decompose
Draft

refactor: extract CSVImport._decompose_site_role, inverse of _evaluate_site_role#1812
jacalata wants to merge 4 commits into
jac/csv-import-fixes-and-find-by-namefrom
jac/csv-site-role-decompose

Conversation

@jacalata

@jacalata jacalata commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Draft while stacked on #1811. Will be marked ready for review once #1811 merges.

Summary

Stacks on #1811.

Moves the ad-hoc site role → CSV columns logic from create_users_csv into UserItem.CSVImport._decompose_site_role, making it the explicit inverse of _evaluate_site_role. Both encode/decode functions now live together in CSVImport.

What changed

New method: UserItem.CSVImport._decompose_site_role(site_role) -> (license, admin_level, publish)

Replaces the if/elif block in create_users_csv with a lookup table. create_users_csv becomes:

license, admin_level, publish = UserItem.CSVImport._decompose_site_role(user.site_role or "Unlicensed")

Pre-existing bugs fixed in the decomposition:

  • ExplorerCanPublish was emitted as license="ExplorerCanPublish" — not a valid CSV license value; now ("Explorer", "None", "1")
  • SiteAdministrator (legacy role) was emitting license="" via str.replace("SiteAdministrator", ""); now maps to ("Explorer", "Site", "1")
  • Non-admin roles now emit admin_level="None" (explicit CSV spec value) rather than "" — both accepted by the server but "None" is consistent with the spec and what _evaluate_site_role expects as input

Test updated: test_create_users_csv now asserts "None" for non-admin roles (previously asserted "") and verifies the auth column.

Test plan

  • pytest test/test_user.py test/test_user_model.py — 46 passed
  • Full suite — 848 passed, 1 skipped

🤖 Generated with Claude Code

@bcantoni

Copy link
Copy Markdown
Contributor

Correctness: silent behavior change for several Roles values

The new _role_map lookup table doesn't faithfully preserve the old if/elif logic for every UserItem.Roles value. Diffing old vs. new decomposition for each role constant:

Role Old CSV output (license, admin, publish) New CSV output
Interactor ("Interactor", "", 1) ("Explorer", "None", "0")
Publisher ("Publisher", "", 1) ("Explorer", "None", "1")
ReadOnly ("ReadOnly", "", 0) ("Viewer", "None", "0")
UnlicensedWithPublish ("UnlicensedWithPublish", "", 1) ("Unlicensed", "None", "0")falls into the .get() default, silently drops publish
ViewerWithPublish ("ViewerWithPublish", "", 1) ("Unlicensed", "None", "0")same silent drop
Guest ("Guest", "", 0) ("Unlicensed", "None", "0")
SupportUser ("SupportUser", "", 0) ("Unlicensed", "None", "0")

Some of these (Interactor, Publisher, ReadOnly) look like reasonable improvements — their old outputs already emitted invalid license strings, so mapping them to real license values is a fix. But UnlicensedWithPublish and ViewerWithPublish look like a real regression: the old code at least preserved publish=1 even though license was garbage. The new _role_map.get(..., default) silently coerces both to Unlicensed/publish=0. Any caller using site_role = UserItem.Roles.ViewerWithPublish will now get a user provisioned as fully unlicensed instead of a licensed publisher, with no error or warning.

Suggestion: add explicit entries for Guest, SupportUser, UnlicensedWithPublish, and ViewerWithPublish to _role_map (matching whatever the CSV import spec actually expects for these), or if they're genuinely unsupported by CSV import, raise/log rather than silently defaulting to Unlicensed. At minimum, worth calling out in the PR description that these four roles' behavior is changing — two of them silently and detrimentally.

@bcantoni

Copy link
Copy Markdown
Contributor

@jacalata take a look at the consistency check that claude called out - does that need to be fixed/improved?

@jacalata
jacalata force-pushed the jac/csv-import-fixes-and-find-by-name branch from d3376c9 to 948e382 Compare July 28, 2026 01:01
jacalata and others added 2 commits July 27, 2026 18:01
…ion check and debug prints

- create_users_csv was producing 7-column CSV, silently dropping
  auth_setting; bulk_add roundtrip would lose auth type
- create_from_file extension check used filepath.find("csv") which
  evaluates as falsy only when "csv" is at index 0, letting all other
  paths through; fixed to "csv" not in filepath
- remove two debug print() calls left in create_from_file

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…th _evaluate_site_role

Moves the ad-hoc site role → (license, admin_level, publish) logic from
create_users_csv into UserItem.CSVImport._decompose_site_role, making it
the explicit inverse of _evaluate_site_role.

Also fixes pre-existing bugs in the decomposition:
- ExplorerCanPublish was emitted as license="ExplorerCanPublish" (not a
  valid CSV license value); now correctly "Explorer" with publish=1
- SiteAdministrator (legacy role) was emitting license="" via
  str.replace; now maps to ("Explorer", "Site", "1")
- Non-admin roles now emit admin_level="None" (explicit CSV spec value)
  rather than "" (empty string); both are accepted by the server but
  "None" is consistent with the spec and _evaluate_site_role input

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jacalata
jacalata force-pushed the jac/csv-site-role-decompose branch from 6402e1d to 7d4522d Compare July 28, 2026 01:01
…ensed"

Legacy values in UserItem.Roles (UnlicensedWithPublish, ViewerWithPublish,
Guest, SupportUser) have never been accepted by the server-side CSV license
parser (workgroup: CsvLicenseRoleTypeConverter). The initial _decompose_site_role
default of ("Unlicensed", "None", "0") silently coerced these to a valid but
semantically wrong Unlicensed user, replacing an old server-side per-row
rejection with silent success.

Default to license="Invalid" instead so the server continues to reject rows
with USER_CSV_INVALID_LICENSE, preserving the pre-refactor observable
behavior for callers who inspect job results for per-row failures. Batch
resilience is unaffected: bad rows fail, good rows succeed, no client-side
throw takes down the whole bulk_add call.

Follow-up: deprecate UnlicensedWithPublish/ViewerWithPublish from
UserItem.Roles (never worked on any code path); see forthcoming issue.
@jacalata

jacalata commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Good question. Several of the old values were actually broken already, and I was under the impression that the server was returning a new Unlicensed user. However it turns out it was throwing an error and not creating an account at all, so Claude was correct that we were changing from an error to a silent not-quite-as-expected success.

so, the old behavior: The server's parser (CsvLicenseRoleTypeConverter) accepts only creator / interactor / explorer / viewer / unlicensed / empty. Anything else throws USER_CSV_INVALID_LICENSE and the row is rejected.
However the roles 'Guest' and 'SupportUser' were accepted when calling the individual users.add method, just not in CSVs: that's a server-side asymmetry between the two endpoints, not a TSC bug. They will still work in users.add. We might try and clean that up as part of future work.

Fix: unmapped roles now emit license="Invalid", which the server rejects with the same USER_CSV_INVALID_LICENSE error you got before. Per-row failure semantics preserved for bulk_add callers who inspect the job result; batch resilience unaffected (bad rows fail, good rows succeed, no client-side throw takes down the whole call).

The legacy Roles values (UnlicensedWithPublish, ViewerWithPublish) didn't work at all. They've been in UserItem.Roles since the first commit of the library in 2016 and appear to have been fossilized dead code since Tableau Server moved off the old license model. Filed #1834 to formally deprecate them.

@jacalata
jacalata marked this pull request as draft July 28, 2026 07:19
The role map handles legacy UserItem.Roles values in two ways depending
on whether the server has a modern equivalent. SiteAdministrator,
Publisher, Interactor, and ReadOnly map to their current-model
equivalents; UnlicensedWithPublish, ViewerWithPublish, Guest, and
SupportUser fall to license="Invalid" so the server rejects the row.

Reviewers otherwise read the code and see "why does Publisher get
mapped but Guest doesn't?" - the answer is server behavior, not
arbitrary choice. Docstring now says so.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants